Added new thread_pool handler #416
Conversation
There was a problem hiding this comment.
@danielmorell Thank you for the thoughtful analysis. It's been a pleasure to review this PR.
Regarding version compatibility, I think the difference in max workers for 3.5 and higher is fine since it is based on the python version the user chose, and is consistent with other uses of ThreadPoolExecutor in the host application. They can always override if they have a better idea.
For python < 3.5, maybe we should set a reasonable default value. It's OK for this to be a different default than other versions as long as it's documented in the SDK docs. If we don't, we still need to suggest a reasonable value in the doc, so this requires some additional documentation regardless.
Good call on the beta version and keeping the current default handler for now.
This is approved as is, but feel free to update with the version dependent default if you agree.
👍
|
@waltjones I appreciate the feedback. Your input has been very helpful! I agree that adding a default for Python < 3.5 would be helpful. I added a new commit e14af00 with the same default as Python 3.5. I will make sure the details are added to the documentation once we publish the release. I will also probably update the explanation of the "thread" handler in the documentation, so it is clear. Let me know if you have any further thoughts. |
|
@danielmorell Looks good, and thank you for the doc improvements! |
Description of the change
Here are the high-level changes...
thread_poolthat utilizes a pool of worker threads to send message to the Rollbar services asynchronously off the main thread.thread_pool_workersto allow users control of the number of worker threads.0.16.4beta. As this may need some tweaks/modifications before we go stable.Now all the considerations...
First, Python version compatibility. Using this requires a minimum of Python version of 3.2. However, depending on the version the behavior may be slightly different. This is because of how Python calculates the maximum number of workers (if the user did not specify it with
thread_pool_workers). The calculation changed in 3.5 and 3.8 (see the docs on ThreadPoolExecutor for details).Second, stability. I had thought about updating the default handler to use a tread pool instead of creating a new thread for every message (that is what it currently does). However, this seems too early to make such a change. Any new handler should really be put through its paces under real world conditions, and ideally be used in production codebases for some time before we even consider changing the default.
Third, max workers. The default for
thread_pool_workersisNone. This is passed to themax_workersargument forThreadPoolExecutor.__init__(). I don't believe a default was added until Python 3.5. This means that our default may not work on versions 3.2 to 3.4. Users will likely need to explicitly set the number of workers. On more recent versionsNonewill cause Python to calculate it based on the number of CPUs (see the link to the docs above).Are we okay with the default not working on every version, or should we provide a different default on lower versions of Python?
Both the implementation and the tests are pretty minimal. If anyone thinks of anything that should also be tested let me know and I will add it. Feedback is very much appreciated!
Type of change
Related issues
Checklists
Development
Code review